-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add menu component #138
Conversation
32886c0
to
f7a0f57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but should this component being put under components
? it looks like a higher-level components to me.
the implementation of the menu should be put in components I think we can add a PR to put the Setting Menu in the widget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a fully-featured, responsive Menu component with nested submenus, mobile drawer support, and a viewport detection hook.
- Introduces
useIsMobile
hook for mobile breakpoint detection. - Implements Menu, SubMenu, MenuItem, SubMenuItem, MenuDrawer, and MenuContext for flexible menu structures.
- Wires up responsive behavior: Popover on desktop, sliding drawer on mobile.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/index.tsx | Export the new Menu entry point |
src/hooks/useIsMobile.ts | Add hook to detect mobile viewport size |
src/components/Menu/index.ts | Aggregate exports for Menu subcomponents |
src/components/Menu/SubMenuItem.tsx | Render submenu items with labels, description, and suffix |
src/components/Menu/SubMenu.tsx | Implement nested submenu with sliding drawer logic |
src/components/Menu/MenuItem.tsx | Implement menu item with click handling and context-based close |
src/components/Menu/MenuDrawer.tsx | Sliding drawer component with header and backdrop |
src/components/Menu/MenuContext.tsx | Create React context for menu state management |
src/components/Menu/Menu.tsx | Main Menu component combining Popover and mobile drawer |
src/components/Menu/Menu.stories.tsx | Add Storybook example demonstrating default menu |
Comments suppressed due to low confidence (2)
src/components/Menu/SubMenu.tsx:10
- Prop naming is inconsistent: SubMenu uses
name
while SubMenuItem useslabel
for primary text. Consider unifying these prop names across Menu components for API consistency.
name?: string; // This is the recommended property for the primary text in menu items. Use this instead of older alternatives.
src/hooks/useIsMobile.ts:29
- The
useIsMobile
hook currently lacks unit tests. Consider adding tests to verify correct behavior across different window widths and resize events.
return isMobile;
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
5e49859
to
6735c7c
Compare
@jeremy-babylonlabs it's quite hard to review what's been changed since last approval Could u squash later commits or just push a single commit once ready? |
@gbarkhatov @jonybur plz double check if there are more comments. |
Uh oh!
There was an error while loading. Please reload this page.